Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new version for proactive detection config #3224

Closed
wants to merge 7 commits into from

Conversation

aviled
Copy link
Contributor

@aviled aviled commented Jun 13, 2018

No description provided.

@AutorestCI
Copy link

AutorestCI commented Jun 13, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Jun 13, 2018

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@AutorestCI
Copy link

AutorestCI commented Jun 13, 2018

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

@AutorestCI
Copy link

AutorestCI commented Jun 13, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3044

@AutorestCI
Copy link

AutorestCI commented Jun 13, 2018

Automation for azure-sdk-for-go

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-go#2353

Copy link
Member

@anuchandy anuchandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviled there is CI failure - with error message DuplicateModelCollsion -. This detects invalid SDK situation that we weren’t detecting before: if two files that are in the same “tag” in the Readme defines the same model name Foo, both object have to be exactly (at the comma exactly level) the same. Here we have Resource model defined in webTests_API.json, components_API.json and workbooks_API.json, it seems these 3 definitions are not same. could you fix it?

@anuchandy
Copy link
Member

@aviled can you fix the error reported by CI (see previous comments) ?

@AutorestCI
Copy link

AutorestCI commented Jun 21, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

Copy link
Member

@anuchandy anuchandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please model patch payload correctly as described in the email

@anuchandy
Copy link
Member

Looking in Eric Chong @ericc1103 owner of workbooks_api-json spec to address ARM violation.

ARM-Violation: "Properties of a PATCH request body must not be required. PATCH operation: 'Workbook_Update' Model Definition: 'WorkbookResource' Property: 'location'"

'WorkbookResource' model inherits from ‘Resource’ (for which we marked location as required), this model is used for both PATCH and PUT. It is required that all the properties of model representing PATCH operation has to be optional. This also indirectly means service should not enforce any requiredness in the PATCH payload.

In-order to fix this, we need to define a new model representing Workbook PATCH payload (with all properties optional). The PATCH payload model looks something like WorkbookUpdateParameters as shown below. But please note that only you know what are the PATCH-able properties of workbook so you will need to update (by adding or removing properties) the below model accordingly.

    "WorkbookUpdateParameters": {
        "properties": {
            "kind": {
                "type": "string",
                "description": "The kind of workbook. Choices are user and shared.",
                "enum": [
                    "user",
                    "shared"
                ],
                "x-ms-enum": {
                    "name": "SharedTypeKind",
                    "modelAsString": true
                }
            },
            "tags": {
                "type": "object",
                "additionalProperties": {
                    "type": "string"
                },
                "description": "Resource tags"
            },
            "properties": {
                "x-ms-client-flatten": true,
                "description": "Metadata describing a web test for an Azure resource.",
                "$ref": "#/definitions/WorkbookPropertiesUpdateParameters"
            }
        },
        "description": "The parameters that can be provided when updating workbook properties properties."
    },
    "WorkbookPropertiesUpdateParameters": {
        "description": "Properties that contain a workbook.",
        "properties": {
          "name": {
            "type": "string",
            "description": "The user-defined name of the workbook."
          },
          "serializedData": {
            "type": "string",
            "description": "Configuration of this particular workbook. Configuration data is a string containing valid JSON"
          },
          "version": {
            "type": "string",
            "description": "This instance's version of the data model. This can change as new features are added that can be marked workbook."
          },
          "workbookId": {
            "type": "string",
            "description": "Internally assigned unique id of the workbook definition."
          },
          "kind": {
           "description": "Enum indicating if this workbook definition is owned by a specific user or is shared between all users with access to the Application Insights component.",
            "x-ms-client-name": "SharedTypeKind",
            "default": "shared",
            "type": "string",
            "enum": [
              "shared",
              "user"
            ],
            "x-ms-enum": {
              "name": "SharedTypeKind",
              "modelAsString": true
            }
          },
          "tags": {
            "type": "array",
            "items": {
              "type": "string"
            },
            "description": "A list of 0 or more tags that are associated with this workbook definition"
          },
          "userId": {
            "type": "string",
            "description": "Unique user id of the specific user that owns this workbook."
          },
          "sourceResourceId": {
            "type": "string",
            "description": "Optional resourceId for a source resource."
          }
        }
      }

@ericc1103
Copy link
Contributor

I have updated mine to make PATCH operation of properties as not required. The PR is @ #2950

@anuchandy
Copy link
Member

@ericc1103 thanks, your PR is for new preview api-version. The issue here is with 2015-05-01 version of workbooks. I replied to your email and clarified, please take a look.

"$ref": "#/parameters/ResourceNameParameter"
}
],
"responses": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a default response to capture the error responses. The schema should match the ARm error contract schema. Example can be found in Batch RP swagger.

"readOnly": true,
"description": "Azure resource Id"
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also be readonly as it comes from the URL

"description": "A ProactiveDetection configuration definition.",
"type": "object",
"x-ms-azure-resource": true,
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tags" ARM top level property missing? Is this a tracked or proxy resource? Seems tracked since it has a location property

"description": "Properties that define a ProactiveDetection configuration.",
"type": "object",
"properties": {
"Name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this name different from the one outside of properties? If different, please call this something else like ruleName

"readOnly": false,
"description": "The rule name"
},
"Enabled": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bools are typically not recommended. String enums generally work better.

"readOnly": false,
"description": "A flag that indicates whether this rule is enabled by the user"
},
"SendEmailsToSubscriptionOwners": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

"type": "string"
}
},
"LastUpdatedTime": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly should be true

}
},
"paths": {
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Insights/components/{resourceName}/ProactiveDetectionConfigs":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete API missing. If this is a tracked resource type, patch also MUST be supported atleast for the update of tags but preferably to allow update of any patachable properties.

@veronicagg
Copy link
Contributor

@aviled any thoughts on @ravbhatnagar 's comments?

@veronicagg
Copy link
Contributor

@aviled any updates? If I don't hear back in the next day, we'll be closing the PR and you can reopen it when you're back to it. Thanks!

@veronicagg
Copy link
Contributor

Closing due to inactivity, please reopen/comment when you're back to this. Thanks.

@veronicagg veronicagg closed this Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants